LCORE-1839: E2E for HITL#2115
Conversation
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds e2e approval configurations for library and server modes, plus scenarios covering MCP approval policies, approve/deny decisions, REST retrieval, expiry, retention cleanup, and invalid approval IDs. ChangesMCP approval end-to-end coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LCS
participant MCP
participant ApprovalAPI
Client->>LCS: query or streaming_query
LCS->>MCP: invoke MCP tool
MCP-->>LCS: approval-required tool request
LCS-->>Client: pending approval id
Client->>ApprovalAPI: approve or deny approval
ApprovalAPI-->>Client: decision result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-retention.yaml`:
- Line 29: The test configuration uses unsupported approval_retention_seconds,
so replace it with a valid configuration field or add a seconds-based retention
option to ApprovalsConfiguration and expose it through ConfigurationBase
validation; update the short-retention purge scenarios to use that supported
setting while preserving their intended timing.
In `@tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals.yaml`:
- Around line 42-46: Replace the <PLACEHOLDER> entries under
require_approval.always and require_approval.never in the MCP approvals
configuration with the actual mock-mcp tool identifiers expected by the granular
scenarios in approvals.feature, assigning each tool to the appropriate approval
list.
In `@tests/e2e/features/approvals.feature`:
- Around line 89-98: Add “When I wait for the response to be completed” after
each streaming_query request in the three affected scenarios: the granular
“always” scenario, the streaming approve scenario, and the streaming deny
scenario. Place the step before all response status, body, and approval ID
assertions, matching the analogous completed streaming scenarios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fdb4d54-1107-4c7d-bc8e-404832118267
📒 Files selected for processing (7)
tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-retention.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-timeout.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-retention.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-timeout.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals.yamltests/e2e/features/approvals.feature
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: integration_tests (3.12)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
- GitHub Check: E2E: library mode / ci / group 2
- GitHub Check: E2E Tests for Lightspeed Evaluation job
⚠️ CI failures not shown inline (1)
GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request: Failed
Conclusion: failure
Konflux kflux-prd-rh02/lightspeed-stack-on-pull-request has <b>failed</b>.
<ul>
<li><b>Namespace</b>: <a href="https://konflux-ui.apps.kflux-prd-rh02.0fk9.p1.openshiftapps.com/ns/lightspeed-core-tenant">lightspeed-core-tenant</a></li>
<li><b>PipelineRun:</b> <a href="https://konflux-ui.apps.kflux-prd-rh02.0fk9.p1.openshiftapps.com/ns/lightspeed-core-tenant/pipelinerun/lightspeed-stack-on-pull-request-h295f">lightspeed-stack-on-pull-request-h295f</a></li>
</ul>
<hr>
<h4>Task Statuses:</h4>
<table>
<tr><th>Status</th><th>Duration</th><th>Name</th></tr>
<tr>
<td>🔴 Failed</td>
<td>0 seconds</td><td>
[init](https://konflux-ui.apps.kflux-prd-rh02.0fk9.p1.openshiftapps.com/ns/lightspeed-core-tenant/pipelinerun/lightspeed-stack-on-pull-request-h295f/logs/init)
</td></tr>
</table>
🧰 Additional context used
📓 Path-based instructions (2)
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Flag meaningful O(n^2)+ algorithms on non-trivial inputs, including handlers and Kubernetes list operations.
Flag N+1 patterns that list items and then query once per item, including Kubernetes API and database access.
Flag expensive work inside loops, including API calls, JSON parsing, and regex compilation.
Flag unbounded growth in caches, watchers, or buffers when eviction or limits are missing.
Flag missing pagination or limits on list operations and API endpoints.
Flag secrets or tokens logged in plaintext or hardcoded in source.
Flag API endpoints missing authentication or authorization.
Flag injection vulnerabilities, including SQL injection, command injection, and path traversal.
Flag sensitive data leaked in API responses, WebSocket messages, or logs.
Flag Kubernetes Secrets and Red Hat secrets missing OwnerReferences.
Files:
tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-timeout.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-timeout.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-retention.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-retention.yamltests/e2e/features/approvals.feature
tests/e2e/**/*.{py,feature}
📄 CodeRabbit inference engine (AGENTS.md)
Use behave (BDD) framework for end-to-end testing with Gherkin feature files
Files:
tests/e2e/features/approvals.feature
🧠 Learnings (1)
📚 Learning: 2026-05-20T08:09:30.641Z
Learnt from: max-svistunov
Repo: lightspeed-core/lightspeed-stack PR: 1580
File: docs/design/llama-stack-config-merge/poc-results/library-mode/synthesized-run.yaml:107-110
Timestamp: 2026-05-20T08:09:30.641Z
Learning: In Llama-stack config YAMLs, when defining a Llama Guard safety shield entry, set `provider_shield_id` to the *guard model identifier* (e.g., `meta-llama/Llama-Guard-3-8B`). Do not use a chat/generative model id (e.g., `openai/gpt-4o-mini`): a chat-model id (or `native_override`) indicates only an override landed and does **not** mean the safety shield is actually gating queries. Ensure any E2E coverage for the related implementation (JIRA/E2E tests) exercises a real Llama Guard model to verify that the shield is effective.
Applied to files:
tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-timeout.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-timeout.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals.yamltests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-retention.yamltests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-retention.yaml
🔇 Additional comments (9)
tests/e2e/features/approvals.feature (3)
13-34: LGTM!Also applies to: 39-49, 66-87, 115-131, 154-170, 213-227, 231-267, 331-352, 355-372
269-329: 🩺 Stability & AvailabilityNo config mismatch here
Bothlightspeed-stack-mcp-approvals-short-retention.yamlandlightspeed-stack-mcp-approvals-short-timeout.yamlsetapproval_retention_seconds: 5, so the 6s/11s waits are enough for the purge scenarios.> Likely an incorrect or invalid review comment.
193-207: 🚀 Performance & ScalabilityGET /approvals needs pagination coverage. This scenario only creates one pending approval, so it doesn’t exercise multi-item results or any page/limit behavior. Add a case with multiple approvals if the endpoint is meant to return a collection.
tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-timeout.yaml (1)
29-29: Sameapproval_retention_secondsschema mismatch aslightspeed-stack-mcp-approvals-short-retention.yamlline 29.This config also uses the non-existent
approval_retention_seconds: 5field. The "Expired approval is purged after retention period" scenario (waits 11s, expects 404) depends on both the 5s timeout and 5s retention to produce a 404 — with the 30-day default, the expired approval would still be retained and the test would fail.tests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals.yaml (2)
43-47: Same<PLACEHOLDER>tool names in granular approval filter as library-mode config.The server-mode granular MCP server has the same placeholder tool names that won't match any real tools.
28-30: LGTM!tests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-retention.yaml (1)
30-30: Sameapproval_retention_secondsschema mismatch as library-mode short-retention config.tests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals-short-timeout.yaml (1)
30-30: Sameapproval_retention_secondsschema mismatch as library-mode short-timeout config.tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals.yaml (1)
27-29: LGTM!
| default_model: gpt-4o-mini | ||
| approvals: | ||
| approval_timeout_seconds: 60 | ||
| approval_retention_seconds: 5 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ApprovalsConfiguration schema for approval_retention_seconds
rg -n -A 30 'class ApprovalsConfiguration' src/models/config.py
# Search for approval_retention_seconds anywhere in the codebase
rg -rn 'approval_retention_seconds' src/
# Check ConfigurationBase extra-field handling
rg -n 'class ConfigurationBase' src/models/config.py
rg -n 'extra|model_config|ConfigDict' src/models/config.py | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 1925
approval_retention_seconds is invalid here. src/models/config.py only defines approval_timeout_seconds and approval_retention_days, and ConfigurationBase forbids extra fields. This config will either fail validation at startup or fall back to the 30-day default, so the 6-second purge scenarios won’t work as written. If short-retention e2e coverage is required, add a supported seconds-based retention setting to ApprovalsConfiguration instead of using an unsupported key.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals-short-retention.yaml`
at line 29, The test configuration uses unsupported approval_retention_seconds,
so replace it with a valid configuration field or add a seconds-based retention
option to ApprovalsConfiguration and expose it through ConfigurationBase
validation; update the short-retention purge scenarios to use that supported
setting while preserving their intended timing.
| require_approval: | ||
| always: | ||
| - "<PLACEHOLDER>" | ||
| never: | ||
| - "<PLACEHOLDER>" No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for granular approval scenarios in the feature file
rg -n 'granular' tests/e2e/features/approvals.feature
# Check what tools the mock MCP server exposes
rg -rn 'mock-mcp' tests/e2e/ --type yaml | head -20Repository: lightspeed-core/lightspeed-stack
Length of output: 3150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- approvals.feature granular section ---'
sed -n '60,115p' tests/e2e/features/approvals.feature
echo
echo '--- mcp-related config files mentioning tool names or placeholders ---'
rg -n '<PLACEHOLDER>|tool|tools|mcp-approval-granular|mock-mcp' tests/e2e/configuration tests/e2e/features -g '*.yaml' -g '*.feature' -g '*.json' -g '*.ts' -g '*.js' | sed -n '1,220p'Repository: lightspeed-core/lightspeed-stack
Length of output: 29040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- server-mode approvals config around granular section ---'
sed -n '30,55p' tests/e2e/configuration/server-mode/lightspeed-stack-mcp-approvals.yaml
echo
echo '--- all occurrences of mcp-approval-granular ---'
rg -n 'mcp-approval-granular' tests/e2e -g '*.yaml' -g '*.feature' -g '*.ts' -g '*.js' | sed -n '1,120p'
echo
echo '--- mock MCP server tool definitions ---'
rg -n 'mock-mcp|tool.*name|name: ".*approval|always|never|granular|approval' tests/e2e -g '*.yaml' -g '*.ts' -g '*.js' | sed -n '1,220p'Repository: lightspeed-core/lightspeed-stack
Length of output: 9553
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- mock mcp files ---'
fd -a 'mock-mcp|mcp' tests/e2e -t f | sed -n '1,200p'
echo
echo '--- search for tool definitions and approval-granular identifiers ---'
rg -n 'tool|tools|name: "mcp-approval-granular"|list_tools|call_tool|approval' tests/e2e -g '*.ts' -g '*.js' -g '*.yaml' -g '*.json' | sed -n '1,240p'Repository: lightspeed-core/lightspeed-stack
Length of output: 9816
Replace the granular approval placeholders with real tool identifiers.
tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals.yaml:42-46 still uses <PLACEHOLDER> for both always and never, so the granular approval scenarios in tests/e2e/features/approvals.feature won’t exercise the intended approval behavior until these lists point at actual mock-mcp tool names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/configuration/library-mode/lightspeed-stack-mcp-approvals.yaml`
around lines 42 - 46, Replace the <PLACEHOLDER> entries under
require_approval.always and require_approval.never in the MCP approvals
configuration with the actual mock-mcp tool identifiers expected by the granular
scenarios in approvals.feature, assigning each tool to the appropriate approval
list.
| Scenario: Streaming query with granular approval filter returns approval_required for "always" tool | ||
| Given MCP toolgroups are reset for a new MCP configuration | ||
| And The service uses the lightspeed-stack-mcp-approvals.yaml configuration | ||
| And The service is restarted | ||
| When I use "streaming_query" to ask question with authorization header | ||
| """ | ||
| {"query": "<PLACEHOLDER: prompt to trigger 'mcp-approval-granular' always tool>", "model": "{MODEL}", "provider": "{PROVIDER}"} | ||
| """ | ||
| Then The status code of the response is 200 | ||
| And The body of the response contains approval_required |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing When I wait for the response to be completed in streaming scenarios.
Three streaming scenarios omit the wait step before asserting on the response body, while the analogous scenarios at lines 51–61 and 100–110 include it:
- Lines 89–98 (streaming granular "always"): No wait before
Then The body of the response contains approval_required. Compare with lines 51–61 which test the same assertion and do wait. - Lines 133–149 (streaming approve): No wait before checking
approval_requiredand extracting the approval ID. - Lines 172–188 (streaming deny): Same pattern as above.
Without the wait step, SSE events may not have fully arrived when body assertions execute, leading to flaky failures. Add the wait step after the streaming query and before the Then assertions, consistent with the other streaming scenarios.
Proposed fix for lines 89–98
When I use "streaming_query" to ask question with authorization header
"""
{"query": "<PLACEHOLDER: prompt to trigger 'mcp-approval-granular' always tool>", "model": "{MODEL}", "provider": "{PROVIDER}"}
"""
+ When I wait for the response to be completed
Then The status code of the response is 200
And The body of the response contains approval_requiredProposed fix for lines 133–149
When I use "streaming_query" to ask question with authorization header
"""
{"query": "<PLACEHOLDER: prompt to trigger 'mcp-approval-always' tool>", "model": "{MODEL}", "provider": "{PROVIDER}"}
"""
+ When I wait for the response to be completed
Then The status code of the response is 200
And The body of the response contains approval_requiredProposed fix for lines 172–188
When I use "streaming_query" to ask question with authorization header
"""
{"query": "<PLACEHOLDER: prompt to trigger 'mcp-approval-always' tool>", "model": "{MODEL}", "provider": "{PROVIDER}"}
"""
+ When I wait for the response to be completed
Then The status code of the response is 200
And The body of the response contains approval_requiredAlso applies to: 133-149, 172-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/features/approvals.feature` around lines 89 - 98, Add “When I wait
for the response to be completed” after each streaming_query request in the
three affected scenarios: the granular “always” scenario, the streaming approve
scenario, and the streaming deny scenario. Place the step before all response
status, body, and approval ID assertions, matching the analogous completed
streaming scenarios.
1 participant
Description
Add end-to-end BDD tests for the Human-in-the-Loop (HITL) MCP tool approval workflow. These tests validate the full approval lifecycle including creating approvals via
require_approval: "always"and"never"modes, approving and denying pending approvals, listing approvals, timeout/expiry behavior (410), retention-based cleanup (404 after purge), and 404 handling for non-existent approvals. Tests cover bothqueryandstreaming_queryendpoints across library-mode and server-mode configurations.Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
approvals.featurefile with 17 scenarios covering the HITL approval API surfaceapproval_timeout_secondsandapproval_retention_secondsvaluesI wait for N seconds) validate time-dependent expiry and retention purge behaviorSummary by CodeRabbit